-
Notifications
You must be signed in to change notification settings - Fork 241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add wrap-validation middleware based on Malli #679
Conversation
26cbe6b
to
24353d2
Compare
We should probably tighten the schemas soon. I've had bad experiences in the past using spec, when people let specs be loose for too long, they became concerned that locking them down would break something (E.g., one team only accepts ports as numbers, but another team also accepts them as strings which it parses...) |
...and I'm already reminded of why |
I'm totally fine with using Malli instead of spec, I don't have strong opinions about them.
Isn't it too late already?
You're a bit harsh here. 😄 In the current example, I'm wondering if we should also pin the |
Maybe make validation opt-in instead? That way we could even justify making the schemas stricter than what is currently implemented (e.g. get rid of the Or perhaps even better: Use defn schemas instead of middleware for this purpose? Then users can choose to enable instrumentation e.g. only for their test suites. This would also unlock static checking via |
Let's take a step back and decide what problem we actually want to solve. The original issue (#162) was lvh complaining about the error message he got when he forgot the method key. What else (if anything) do we want out of validation? Just to return some better error messages for bad requests? To lock down the request format users submit to the client? To capture our current loose schemas, but then transform the input into something more stringent for Aleph's use? Get kondo support for our own use? Personally, I:
|
For me, that should be the focus. |
@DerGuteMoritz Any thoughts on the goal of validation? |
@KingMob Thanks for providing context of the original issue, I was missing that 🙏
Looking at the original issue, that definitely should be the priority. Question is whether this alone justifies pulling in malli, especially if it's only for this particular API. We'd end up with a mixed bag of schema-based and hand-rolled validation (and errors). I feel like adopting a schema-based approach should be done in a more holistic fashion. For #162 I now lean towards a minimally invasive patch with hand-rolled validation.
Agreed.
Agreed.
Indeed. As mentioned above, I think doing this via malli is worthwhile since it will also give us opt-in runtime validation (which could then supercede the hand-rolled one) and generative testing support at the same time. Ideally then for all of Aleph's public API. |
I'd like to pull in Malli as a vote for the future, then, and because kondo support would help me, personally, a lot. We might not need it for any given small PR, but if it's already a dep, we can start to build and rely on it. |
7c88702
to
da7f15a
Compare
Add new aleph.http.schema ns
Add :any body schema
2c7e389
to
651ba92
Compare
Any further thoughts on this? @DerGuteMoritz do you still want changes, or is that stale? |
@arnaudgeiser What do you think of this?
Also moves the schemas to a common aleph.http.schema ns, so they can be shared.
Not sure we need each individual var schema just yet, so I've hidden the ns from cljdoc for now.